Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Log4j simplifications #7651

Merged
merged 5 commits into from
Oct 7, 2023
Merged

Log4j simplifications #7651

merged 5 commits into from
Oct 7, 2023

Conversation

sschuberth
Copy link
Member

@sschuberth sschuberth commented Oct 6, 2023 •
edited
Loading

Please have a look at the individual commit messages for the details.

Note
Strictly speaking, this is a breaking change since Logging was (partly) public API before, so e.g. external plugins could have used it.

@sschuberth sschuberth force-pushed the log4j-simplifications branch 2 times, most recently from 1598914 to 89aeb50 Compare October 6, 2023 20:28
@codecov
Copy link

codecov bot commented Oct 6, 2023 •
edited
Loading

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (1c4c0fc) 68.03% compared to head (e40b8ed) 68.02%.
Report is 2 commits behind head on main.

Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #7651      +/-   ##
============================================
- Coverage     68.03%   68.02%   -0.02%     
  Complexity     2023     2023              
============================================
  Files           344      344              
  Lines         16727    16733       +6     
  Branches       2372     2371       -1     
============================================
+ Hits          11381    11383       +2     
- Misses         4363     4367       +4     
  Partials        983      983
Flag Coverage Δ
funTest-non-docker 36.45% <25.00%> (ø)
test 35.57% <23.07%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
advisor/src/main/kotlin/advisors/GitHubDefects.kt 89.07% <ø> (ø)
advisor/src/main/kotlin/advisors/NexusIq.kt 0.00% <ø> (ø)
advisor/src/main/kotlin/advisors/OssIndex.kt 84.00% <ø> (ø)
advisor/src/main/kotlin/advisors/VulnerableCode.kt 83.33% <ø> (ø)
analyzer/src/main/kotlin/managers/GoDep.kt 83.84% <ø> (ø)
analyzer/src/main/kotlin/managers/GoMod.kt 79.59% <ø> (ø)
analyzer/src/main/kotlin/managers/Maven.kt 80.32% <ø> (ø)
analyzer/src/main/kotlin/managers/utils/Graph.kt 86.36% <ø> (ø)
...in/kotlin/managers/utils/MavenDependencyHandler.kt 100.00% <ø> (ø)
...yzer/src/main/kotlin/managers/utils/MavenLogger.kt 23.52% <ø> (ø)
... and 41 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sschuberth sschuberth force-pushed the log4j-simplifications branch from 89aeb50 to 0c8f6a1 Compare October 6, 2023 21:10
@sschuberth
refactor: Avoid the logger to leak into the public API
8e475b1 
Use the new Log4j Kotlin API extension property to avoid the logger to
leak into the public API via (public) companion objects. Also, find a
better way to log from top-level functions by using
`MethodHandles.lookup()` (introduced in JDK 7) to get the name of the
Kotlin-internal class that is generated to hold top-level functions.
SLF4J describes this as an idiomatic way to get a logger, see [1].

[1]: https://www.slf4j.org/faq.html#declaration_pattern

Signed-off-by: Sebastian Schuberth <[email protected]>
@sschuberth
docs(MavenLogger): Correct a comment about the logger forwarded to
8b79956 
Signed-off-by: Sebastian Schuberth <[email protected]>
@sschuberth
chore(GitLabLicenseModelMapper): Slightly improve a log message
0f01456 
Signed-off-by: Sebastian Schuberth <[email protected]>
@sschuberth sschuberth force-pushed the log4j-simplifications branch from 0c8f6a1 to e5c1e88 Compare October 6, 2023 21:14
@sschuberth
chore: Make OkHttpClientHelper the first class in the file
e40b8ed 
`OkHttpClientHelper` should go first as the file is named after it.

Signed-off-by: Sebastian Schuberth <[email protected]>
@sschuberth sschuberth force-pushed the log4j-simplifications branch from e5c1e88 to e40b8ed Compare October 7, 2023 11:24
@sschuberth sschuberth mentioned this pull request Oct 7, 2023
Closed
1 task
@sschuberth sschuberth enabled auto-merge (rebase) October 7, 2023 11:46
@sschuberth sschuberth merged commit 230b550 into main Oct 7, 2023
21 of 22 checks passed
@sschuberth sschuberth deleted the log4j-simplifications branch October 7, 2023 12:12
@nnobelis
Copy link
Member

IMO this is a breaking change: since Logging was public API before, external plugins could have used it.

@sschuberth
Copy link
Member Author

IMO this is a breaking change: since Logging was public API before, external plugins could have used it.

Yes, right. For the PRs / version bumping it's too late, but I'll at least label this PR accordingly.

@nnobelis
Copy link
Member

@sschuberth: Before this PR, the loggers were throwing exception if the calling classes were not in org.ossreviewtoolkit..
Therefore, the plugins used to piggyback on an ORT class logger to output their messages.

Now, they cannot use ORT classes' loggers but it should not be a problem since the aforementioned exception is not thrown anymore. Right ?

@sschuberth
Copy link
Member Author

sschuberth commented Oct 16, 2023 •
edited
Loading

Before this PR, the loggers were throwing exception if the calling classes were not in org.ossreviewtoolkit..

I don't believe that's true. That code was already removed way back in 606da3a.

However, I acknowledge that this PR reintroduces a behavior that 606da3a circumvented, which is to "Stop polluting Any object with a logger property". This "pollution" is now back again (for files that import the extension property), but by now I actually regard this to be less of an issue, as also with the previous code logging on the wrong class could haven happened by accidentally using the base class's logger in derived classes.

since the aforementioned exception is not thrown anymore. Right ?

The exception is not thrown anymore in any case.

@nnobelis
Copy link
Member

Understood thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnonnenmacher mnonnenmacher mnonnenmacher approved these changes

@MarcelBochtler MarcelBochtler Awaiting requested review from MarcelBochtler MarcelBochtler is a code owner

@oheger-bosch oheger-bosch Awaiting requested review from oheger-bosch oheger-bosch is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sschuberth @nnobelis @mnonnenmacher